Skip to content

sec (7/9): redact only declared scanner secretEnv values#31

Merged
jesse-merhi merged 1 commit into
mainfrom
sec/07-redact-env-errors
Jul 27, 2026
Merged

sec (7/9): redact only declared scanner secretEnv values#31
jesse-merhi merged 1 commit into
mainfrom
sec/07-redact-env-errors

Conversation

@jesse-merhi

@jesse-merhi jesse-merhi commented Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

New behavior

User-defined scanners now have two explicit environment buckets:

env:
  - URL_SCANNER_TOKEN
secretEnv:
  - SCANNER_AUTH

Both variables are required and passed to the scanner. Only secretEnv values are redacted from scanner failure text. A value placed only in env remains visible even when its name contains TOKEN, so the configuration—not a naming heuristic—controls secrecy.

For example, stderr:

mode visible-value auth credential-sensitive-suffix

is stored as:

mode visible-value auth [redacted]

Valid scanner stdout remains raw JSON evidence and is not rewritten. Scanner authors must not print secrets into that JSON.

This carries the intended env / secretEnv contract from the stale PR 22 onto current main.

Verification

  • focused secretEnv behavior and profile parsing tests — passed
  • go vet ./... — passed
  • go test -count=1 -skip 'TestResolveTargetClassifiesPlugin(Directory|ManifestFile)$' ./... — passed\n\nThe two excluded tests are unrelated existing macOS /var versus /private/var path assertions; they run normally in Linux CI.

Copilot AI review requested due to automatic review settings July 23, 2026 12:59
Copilot AI reviewed Jul 23, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

This was referenced Jul 24, 2026
Merged
Merged
Merged
@jesse-merhi
jesse-merhi force-pushed the sec/06-isolated-cwd branch from 494f412 to 1774bc0 Compare July 26, 2026 02:36
@jesse-merhi
jesse-merhi force-pushed the sec/07-redact-env-errors branch from dfd5ec2 to 17876cf Compare July 26, 2026 02:37
@jesse-merhi
jesse-merhi changed the base branch from sec/06-isolated-cwd to main July 26, 2026 02:37
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P1 Urgent regression or broken agent/channel workflow affecting real users now. labels Jul 26, 2026
@clawsweeper

clawsweeper Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed July 26, 2026, 11:58 PM ET / July 27, 2026, 03:58 UTC.

ClawSweeper review

What this changes

This PR adds a secretEnv profile field for user-defined scanners and redacts only values declared there from failed-command error text, while leaving raw scanner JSON unchanged.

Merge readiness

⚠️ Ready for maintainer review - 5 items remain

Keep this PR open for a security-boundary and upgrade-safety correction. The new explicit secretEnv contract is clear for new profiles, but the changed error path would expose secret-looking values from existing user-defined scanner env entries that current main redacts by name; this needs maintainer agreement and a compatibility-preserving implementation before merge.

Priority: P1
Reviewed head: 6268517a57e968cb4ea7a1adce55fdc1422f26c1
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The parsing, docs, and focused tests are coherent, but a high-confidence upgrade security regression blocks merge readiness.
Proof confidence 🌊 off-meta tidepool Not applicable: This is member-authored rather than an external contributor PR, so the external real-behavior-proof gate does not apply; the supplied focused tests and successful checks are supplemental evidence only.
Patch quality 🦐 gold shrimp (3/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: This is member-authored rather than an external contributor PR, so the external real-behavior-proof gate does not apply; the supplied focused tests and successful checks are supplemental evidence only.
Evidence reviewed 4 items Current-main compatibility baseline: The pre-change user-defined-scanner failure path calls the generic command-error redactor, whose selection is based on isSecretEnvKey; the PR replaces that call with one limited to adapter.config.SecretEnv. Existing profiles introduced by the merged BYOS work can therefore have a *_TOKEN value in env that was redacted on current main and becomes visible after this change.
Established profile usage: The merged user-defined-scanner implementation documented an existing profile shape using env: [SEMGREP_APP_TOKEN]; it was merged as #23. That makes name-based protection of existing env secrets an upgrade concern rather than only a hypothetical new-profile mistake.
Proposed contract and coverage: The PR parses secretEnv, combines it with env for scanner requirements, documents the distinction, and adds focused tests for failure-text redaction and raw-evidence preservation. The tests validate the new contract but do not cover an existing secret-named env declaration retaining its current redaction behavior.
Findings 1 actionable finding [P1] Preserve redaction for existing secret-named env entries
Security Needs attention Existing env credentials can leak into failure artifacts: Only SecretEnv names reach the new redactor, so a previously supported env declaration such as SEMGREP_APP_TOKEN is no longer protected by the generic name-based secret backstop when emitted in command error text.

How this fits together

ClawScan resolves profile-defined scanners into runner adapters, passes their declared environment variables into sandboxed commands, and records either raw JSON evidence or failure text in run artifacts. This PR changes which scanner environment values are removed from that failure text.

flowchart LR
  A[Profile scanner config] --> B[env and secretEnv declarations]
  B --> C[User-defined scanner adapter]
  C --> D[Sandboxed scanner command]
  D --> E[Raw JSON evidence]
  D --> F[Failure stderr and command error]
  B --> G[Redaction selection]
  G --> F
Loading

Decision needed

Question Recommendation
Should existing secret-looking names declared under env keep their current error-text redaction compatibility while secretEnv becomes the explicit always-redacted category? Preserve legacy redaction: Redact declared secretEnv values and continue redacting secret-looking legacy env names, with regression tests for existing profile configurations.

Why: The PR intentionally changes a credential-redaction boundary for already-valid user profiles, so maintainers must choose whether explicit configuration may silently replace the current name-based safety backstop.

Before merge

  • Preserve redaction for existing secret-named env entries (P1) - This replaces the current generic redactor with a list containing only SecretEnv. Existing user-defined scanner profiles already declare values such as SEMGREP_APP_TOKEN under env; when their command fails and echoes the value, this branch now writes it into the error artifact even though current main redacts it by name. Keep the legacy secret-name backstop for env (while always redacting secretEnv) or provide an explicit, approved migration path.
  • Resolve security concern: Existing env credentials can leak into failure artifacts - Only SecretEnv names reach the new redactor, so a previously supported env declaration such as SEMGREP_APP_TOKEN is no longer protected by the generic name-based secret backstop when emitted in command error text.
  • Resolve merge risk (P1) - Merging this change can place an existing env value such as SEMGREP_APP_TOKEN into scanner failure artifacts when the scanner exits unsuccessfully, even though current main redacts secret-looking environment names; operators may not discover the regression until a failed scan exposes a credential.
  • Complete next step (P2) - A maintainer must choose the upgrade secrecy contract before a mechanical repair can safely change the redaction rule.

Findings

  • [P1] Preserve redaction for existing secret-named env entries — internal/runner/user_defined_scanner.go:116
  • [high] Existing env credentials can leak into failure artifacts — internal/runner/user_defined_scanner.go:116
Agent review details

Security

Needs attention: The patch narrows an existing credential-redaction boundary and can expose token-like values from pre-existing scanner profile configuration.

Review metrics

Metric Value Why it matters
Patch surface 6 files affected; 225 additions, 40 deletions The change spans profile parsing, command execution, tests, and operator documentation, so the secrecy contract must remain consistent end to end.
Validation coverage 127 test lines added across 2 Go test files The patch has focused coverage, but it omits the legacy secret-named env upgrade case.

Merge-risk options

Maintainer options:

  1. Retain the legacy secret-name backstop (recommended)
    Before merge, include secret-looking env names in failure-text redaction as well as every secretEnv name, and add an upgrade regression test.
  2. Accept an intentional breaking secrecy change
    Merge only after maintainers explicitly approve that old env credentials may appear in failure artifacts and provide a migration path for profile authors.
  3. Pause pending contract direction
    Hold the PR if the project does not want to commit to either compatibility behavior or a documented breaking migration.

Technical review

Best possible solution:

Keep secretEnv as the explicit always-redacted list, but retain the existing secret-name redaction backstop for legacy env entries until maintainers intentionally define and document a safe migration path; add upgrade regression coverage for both categories.

Do we have a high-confidence way to reproduce the issue?

Yes from source inspection: configure a user-defined scanner with env: [SEMGREP_APP_TOKEN], set that value, and have the command emit it on stderr while failing. Current main's generic error path selects secret-looking names for redaction, whereas this PR passes only secretEnv names.

Is this the best way to solve the issue?

No. Explicit secretEnv is a useful improvement, but removing the existing secret-name fallback is not the safest upgrade path because prior profiles already use token-like names under env.

Full review comments:

  • [P1] Preserve redaction for existing secret-named env entries — internal/runner/user_defined_scanner.go:116
    This replaces the current generic redactor with a list containing only SecretEnv. Existing user-defined scanner profiles already declare values such as SEMGREP_APP_TOKEN under env; when their command fails and echoes the value, this branch now writes it into the error artifact even though current main redacts it by name. Keep the legacy secret-name backstop for env (while always redacting secretEnv) or provide an explicit, approved migration path.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 2d4298abcc99.

Labels

Label changes:

  • add merge-risk: 🚨 compatibility: The new redaction rule changes the observable artifact behavior of already-valid env declarations on upgrade.
  • add merge-risk: 🚨 security-boundary: The diff narrows credential redaction in scanner failure text and can disclose values current main protects.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This is member-authored rather than an external contributor PR, so the external real-behavior-proof gate does not apply; the supplied focused tests and successful checks are supplemental evidence only.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: ⏳ waiting on author.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P1: A failed user-defined scanner can expose credentials from existing profiles, making this an urgent security regression in a real execution path.
  • merge-risk: 🚨 compatibility: The new redaction rule changes the observable artifact behavior of already-valid env declarations on upgrade.
  • merge-risk: 🚨 security-boundary: The diff narrows credential redaction in scanner failure text and can disclose values current main protects.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🌊 off-meta tidepool and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: This is member-authored rather than an external contributor PR, so the external real-behavior-proof gate does not apply; the supplied focused tests and successful checks are supplemental evidence only.

Evidence

Security concerns:

  • [high] Existing env credentials can leak into failure artifacts — internal/runner/user_defined_scanner.go:116
    Only SecretEnv names reach the new redactor, so a previously supported env declaration such as SEMGREP_APP_TOKEN is no longer protected by the generic name-based secret backstop when emitted in command error text.
    Confidence: 0.96

What I checked:

Likely related people:

  • jesse-merhi: Authored the merged user-defined-scanner implementation and the merged isolated-working-directory hardening, and authored this follow-up security change in the same execution path. (role: feature introducer and recent area contributor; confidence: high; commits: 3e62e868b938, 93450d72cc7a, 6268517a57e9; files: internal/profiles/resolver.go, internal/runner/user_defined_scanner.go, internal/runner/runner.go)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Restore or deliberately migrate the current secret-name redaction behavior for existing env declarations.
  • Add a focused upgrade test showing a token-like legacy env value remains redacted while a non-secret env value remains visible.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (2 earlier review cycles)
  • reviewed 2026-07-26T02:40:36.786Z sha 17876cf :: needs maintainer review before merge. :: none
  • reviewed 2026-07-26T17:44:17.464Z sha 0300296 :: needs maintainer review before merge. :: none

@clawsweeper clawsweeper Bot mentioned this pull request Jul 26, 2026
Merged
@jesse-merhi
jesse-merhi force-pushed the sec/07-redact-env-errors branch from 17876cf to 0300296 Compare July 26, 2026 17:40
@jesse-merhi
jesse-merhi force-pushed the sec/07-redact-env-errors branch from 0300296 to 6268517 Compare July 27, 2026 03:53
@jesse-merhi jesse-merhi changed the title sec (7/9): redact declared scanner env values from error text sec (7/9): redact only declared scanner secretEnv values Jul 27, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jul 27, 2026
@jesse-merhi
jesse-merhi merged commit 1c247ac into main Jul 27, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments
@Patrick-Erichsen Patrick-Erichsen Awaiting requested review from Patrick-Erichsen Patrick-Erichsen is a code owner

Assignees

No one assigned

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jesse-merhi